Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

test: mark flaky tests as flaky #22330

Closed
wants to merge 2 commits into from
Closed

test: mark flaky tests as flaky #22330

wants to merge 2 commits into from

Conversation

joyeecheung
Copy link
Member

In the last 100 CI runs, these tests have failed too many unrelated PRs

Reason     async-hooks/test-callback-error
Type       JS_TEST_FAILURE
Failed PR  13 (#22266, #22211, #22274, #22289, #22284, #22293, #22300, #22273, #22310, #22249, #22306, #22318, #22269)
Appeared   test-digitalocean-fedora27-x64-1
First CI   https://ci.nodejs.org/job/node-test-pull-request/16383/
Last CI    https://ci.nodejs.org/job/node-test-pull-request/16450/
--------------------------------------------------------------------------------
Reason     parallel/test-cluster-master-error
Type       JS_TEST_FAILURE
Failed PR  10 (#22293, #22307, #22310, #22306, #22249, #22320, #22318, #21611, #22128, #22269)
Appeared   test-joyent-alpine38_container-x64-1, test-softlayer-alpine38_container-x64-1
First CI   https://ci.nodejs.org/job/node-test-pull-request/16434/
Last CI    https://ci.nodejs.org/job/node-test-pull-request/16450/
--------------------------------------------------------------------------------
Reason     parallel/test-cluster-master-kill
Type       JS_TEST_FAILURE
Failed PR  10 (#22293, #22307, #22310, #22306, #22249, #22320, #22318, #21611, #22128, #22269)
Appeared   test-joyent-alpine38_container-x64-1, test-softlayer-alpine38_container-x64-1
First CI   https://ci.nodejs.org/job/node-test-pull-request/16434/
Last CI    https://ci.nodejs.org/job/node-test-pull-request/16450/
--------------------------------------------------------------------------------
Reason     async-hooks/test-statwatcher
Type       JS_TEST_FAILURE
Failed PR  7 (#22274, #22290, #22288, #22293, #22242, #21611, #22302)
Appeared   test-azure_msft-win2016-x64-4, test-joyent-ubuntu1604_sharedlibs_container-x64-1
First CI   https://ci.nodejs.org/job/node-test-pull-request/16387/
Last CI    https://ci.nodejs.org/job/node-test-pull-request/16451/

Mark them as flaky for now so we can land PRs

@nodejs-github-bot
Copy link
Collaborator

@joyeecheung sadly an error occured when I tried to trigger a build :(

@nodejs-github-bot nodejs-github-bot added async_hooks Issues and PRs related to the async hooks subsystem. test Issues and PRs related to the tests. labels Aug 15, 2018
@joyeecheung
Copy link
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/16454/

cc @Trott @nodejs/testing

@joyeecheung joyeecheung added the fast-track PRs that do not need to wait for 48 hours to land. label Aug 15, 2018
@joyeecheung
Copy link
Member Author

Waiting on the container rebuilds

@joyeecheung joyeecheung added the blocked PRs that are blocked by other issues or PRs. label Aug 15, 2018
@rvagg
Copy link
Member

rvagg commented Aug 15, 2018

sorry for the CI interruption with containers. restarted @ https://ci.nodejs.org/job/node-test-pull-request/16456/

Trott
Trott reviewed Aug 15, 2018
View reviewed changes
test/parallel/parallel.status Outdated
@@ -9,6 +9,9 @@ prefix parallel
[$system==win32]

[$system==linux]
# https://github.com/nodejs/node/issues/22308
test-cluster-master-kill: PASS,FLAKY
test-cluster-master-error: PASS,FLAKY
Copy link
Member

@Trott Trott Aug 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The host/platform that these two were failing on has been removed from CI, so I think there's no need to mark them as flaky. (For what it's worth, they were failing 100% of the time and I'm not a fan of marking 100% failures as flaky.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the other hand, I'd like to see Alpine 3.8 back in rotation. We're risking either ignoring Alpine 3.8 completely, or if we mark this as flaky we risk not fixing the underlying problem. Pick your poison.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also against adding this, since it'll create a permanent yellow CI, and that is desensitizing...
We have a tracking issue, and we have eyes on the problem, so the chances of loosing track are lower.

@rvagg
Copy link
Member

rvagg commented Aug 15, 2018

and again https://ci.nodejs.org/job/node-test-pull-request/16457/
and https://ci.nodejs.org/view/All/job/node-test-commit-alpine38/5/ for Alpine 3.8 specifically

@joyeecheung
Copy link
Member Author

Removed the cluster errors. New CI: https://ci.nodejs.org/job/node-test-pull-request/16461/

@joyeecheung joyeecheung removed the blocked PRs that are blocked by other issues or PRs. label Aug 15, 2018
@joyeecheung joyeecheung mentioned this pull request Aug 15, 2018
Closed
@joyeecheung
Copy link
Member Author

joyeecheung commented Aug 15, 2018 •
edited
Loading

All the failures are git failures, so this should be ready to land

Failures in job https://ci.nodejs.org/job/node-test-pull-request/16461/

ubuntu1604-arm64

See failures on test-packetnet-ubuntu1604-arm64-2: 
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

	modified:   deps/v8/third_party/jinja2/LICENSE

Untracked files:
  (use "git add <file>..." to include in what will be committed)

	build/

no changes added to commit

ubuntu1604_sharedlibs_debug_x64

See failures on test-softlayer-ubuntu1604_sharedlibs_container-x64-2: 
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

	modified:   deps/v8/third_party/jinja2/LICENSE

no changes added to commit

ubuntu1604_sharedlibs_openssl110_x64

See failures on test-softlayer-ubuntu1604_sharedlibs_container-x64-5: 
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

	modified:   deps/v8/third_party/jinja2/LICENSE

no changes added to commit

joyeecheung added a commit that referenced this pull request Aug 15, 2018
@joyeecheung
test: mark async-hooks/test-statwatcher as flaky
b87cb5d 
Refs: #21425

PR-URL: #22330
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
joyeecheung added a commit that referenced this pull request Aug 15, 2018
@joyeecheung
test: mark async-hooks/test-callback-error as flaky
59c8abf 
Refs: #15985

PR-URL: #22330
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@joyeecheung
Copy link
Member Author

Landed in b87cb5d and 59c8abf

@Trott
Copy link
Member

Trott commented Aug 15, 2018

All the failures are git failures, so this should be ready to land

Do note that the COLLABORATOR_GUIDE "Testing and CI" section says that those need to be fixed and re-run rather than disregarded and landed:

Do not land any Pull Requests without passing (green or yellow) CI runs. If you believe any failed (red or grey) CI sub-tasks are unrelated to the change in the Pull Request, use "Resume Build" in the left navigation of the relevant node-test-pull-request job. It will create a new node-test-pull-request run that preserves all the green results from the current job but re-runs everything else.

@joyeecheung
Copy link
Member Author

@Trott Ah, sorry about that, although in this case resuming the builds does not help because those are build failures. I am going to fix them in nodejs/build#1453

@joyeecheung joyeecheung mentioned this pull request Aug 17, 2018
Closed
targos pushed a commit that referenced this pull request Aug 19, 2018
@joyeecheung @targos
test: mark async-hooks/test-statwatcher as flaky
d0f236d 
Refs: #21425

PR-URL: #22330
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
targos pushed a commit that referenced this pull request Aug 19, 2018
@joyeecheung @targos
test: mark async-hooks/test-callback-error as flaky
32b5e00 
Refs: #15985

PR-URL: #22330
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
targos pushed a commit that referenced this pull request Sep 3, 2018
@joyeecheung @targos
test: mark async-hooks/test-statwatcher as flaky
4a28d38 
Refs: #21425

PR-URL: #22330
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
targos pushed a commit that referenced this pull request Sep 3, 2018
@joyeecheung @targos
test: mark async-hooks/test-callback-error as flaky
2ed22df 
Refs: #15985

PR-URL: #22330
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@targos targos mentioned this pull request Sep 5, 2018
Merged
This was referenced Sep 6, 2018
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott Trott left review comments

@cjihrig cjihrig cjihrig approved these changes

@rvagg rvagg rvagg approved these changes

@refack refack refack approved these changes

Assignees
No one assigned
Labels
async_hooks Issues and PRs related to the async hooks subsystem. fast-track PRs that do not need to wait for 48 hours to land. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@joyeecheung @nodejs-github-bot @rvagg @Trott @refack @cjihrig